Skip to content

node 4x retry - #74

Closed
erulabs wants to merge 3 commits into
xmppo:masterfrom
erulabs:node4x-alt
Closed

erulabs wants to merge 3 commits into
xmppo:masterfrom
erulabs:node4x-alt

Conversation

@erulabs

@erulabs erulabs commented Sep 28, 2015

Copy link
Copy Markdown

@erulabs

erulabs commented Sep 28, 2015

Copy link
Copy Markdown
Author

@sonnyp Maybe you can take a peek at this one? Should be a lot closer.

I think all the Nan2 stuff is working - but the core tests are failing :/

@erulabs erulabs mentioned this pull request Sep 28, 2015
Closed
@erulabs

erulabs commented Sep 29, 2015

Copy link
Copy Markdown
Author

Hey @sonnyp

Well, I'm close. Very close. Can't figure how why it's not throwing exceptions for fallback JS profiles properly...

I've got to stop working on this now (ran out of time). HUUUGE thanks goes out to anyone who spends some time on this for me :)

Thanks!

Comment thread index.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoa, no, why was this changed?

@rvagg

rvagg commented Sep 29, 2015

Copy link
Copy Markdown
Contributor

You should be able to replace all references to Handle with Local for maximum compatibility

@rvagg

rvagg commented Sep 29, 2015

Copy link
Copy Markdown
Contributor

Handle<Value> prepare(String::Value &str) should have a Nan::HandleScope scope; otherwise it'll leak data (looks like it has been till now anyway) and in this case it should be a Nan::EscapableHandleScope scope because you're returning a value from it.

@rvagg

rvagg commented Sep 29, 2015

Copy link
Copy Markdown
Contributor

https://github.com/nodejs/nan/blob/master/doc/scopes.md#nanescapablehandlescope - you also need to use scope.Escape(obj)

Comment thread node-stringprep.cc

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the scope.Escape(Nan::Undefined())

@rvagg

rvagg commented Sep 29, 2015

Copy link
Copy Markdown
Contributor

I did a write-up @ https://nodesource.com/blog/cpp-addons-for-nodejs-v4 about the upgrade process that's a bit more verbose than other resources currently available

@sonnyp

sonnyp commented Sep 29, 2015

Copy link
Copy Markdown
Contributor

closing this in favor of #72

@erulabs thanks!

@sonnyp sonnyp closed this Sep 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants